Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loans: date range search #783

Merged
merged 2 commits into from
Apr 4, 2020
Merged

Conversation

topless
Copy link
Member

@topless topless commented Mar 27, 2020

  • filters loans_from_date and loans_to_date on loan start date
  • datepickers for ui
  • depends on react-searchkit release 0.18.0
  • closes [Req] Statistics - Loans #168

date-rage-640

@topless topless mentioned this pull request Mar 27, 2020
@topless topless added this to the 2020/W12 milestone Mar 27, 2020
@topless topless requested review from ntarocco, kpsherva and zzacharo and removed request for kpsherva March 27, 2020 02:30
@ntarocco
Copy link
Contributor

Requires: inveniosoftware/react-searchkit#99

@topless topless self-assigned this Mar 30, 2020
@topless topless force-pushed the 168-date-filter branch 5 times, most recently from 73068cd to 7e4ca5d Compare April 1, 2020 00:03
@kpsherva kpsherva modified the milestones: 2020/W12, 2020/W14 Apr 1, 2020
@topless topless force-pushed the 168-date-filter branch 5 times, most recently from c51d111 to f21b380 Compare April 2, 2020 12:53
@@ -30,8 +30,10 @@ beforeEach(() => {
});

const searchApi = new InvenioSearchApi({
url: documentApi.searchBaseURL,
withCredentials: true,
axios: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did it change because of axios or react-searchkit? (sorry if this was mentioned somewhere before)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kprzerwa searchkit :)

Copy link
Contributor

@kpsherva kpsherva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we have a small screenshot of the filter looks like?


/** react-searchkit allows having the same filter multiple times.
* For the range dates filters we want each filter one time only so we have
* to remove any pre-existing filters with the same name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when does it happen that we have filters with the same name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I would re-phrase then given the confusion:

/** react-searchkit allows having the same filter multiple times with different values.
   * For this range dates filters, we want each filter to appear only one time with one value
   * (e.g. loan_start_date = `<date>`)
   *


/** react-searchkit allows having the same filter multiple times.
* For the range dates filters we want each filter one time only so we have
* to remove any pre-existing filters with the same name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I would re-phrase then given the confusion:

/** react-searchkit allows having the same filter multiple times with different values.
   * For this range dates filters, we want each filter to appear only one time with one value
   * (e.g. loan_start_date = `<date>`)
   *

let filters = newFilter;
// If value is empty we simply remove the filter otherwise if we have
// value we remove the filter and add the new one.
if (!_isEmpty(value)) filters = [[name, ''], newFilter];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove the previously selected filter, shouldn't you pass the already selected value first?
E.g. currently I have loan_start_date:2020-01-01 if I select the date loan_start_date:2019-01-01, shouldn't we pass:
[['loan_start_date', '2020-01-01'], ['loan_start_date', '2019-01-01']]

Copy link
Member Author

@topless topless Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was indeed my initial understanding, but in the searchkit the comparison we do not check for the value, we only check if it matches the starting part. So if there is any value we pass ['loan_start_date', ''] to clear it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is not the case, see tests that you wrote too. This is related to this bug: it happens that since the string is empty, it clears out all filters selected for this filter. If we fix that bug, we might break this code.
Please change it to something like (pseudo-code):

onDateChange = newSelectedValue => {
    const resetPreviousValue = getFromRSKCurrentQueryState()
    const newFilters = [resetPreviousValue];
    if (!isEmpty(newSelectedValue)) {
        newFilters.push(newSelectedValue);
    }
    return this.props.updateQueryState({ filters: newFilters });
  };

@topless
Copy link
Member Author

topless commented Apr 2, 2020

could we have a small screenshot of the filter looks like?

Screenshot 2020-04-02 23 27 54

Screenshot 2020-04-02 23 25 41

@topless topless force-pushed the 168-date-filter branch 3 times, most recently from 3a89081 to 032c41b Compare April 3, 2020 10:23
*/
onDateChange = newFilter => {
const [name, value] = newFilter;
if (_isEmpty(value))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this if. If empty, it means that before might had a value, so you should call updateQueryState with the previous value.
If it had no value even before, then just return, nothing to do.
I would simply remove this if. WDYT?

);

let filters = [newFilter];
if (!_isEmpty(oldFilters)) filters.unshift(_flatten(oldFilters));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand all this manipulation of values and different cases.
Here my reasoning:

case 1:

  • current: []
  • newFilter: ['loan_start_date', '2020-01-01']
  • call this.props.updateQueryState({ filters: [ ['loan_start_date', '2020-01-01'] ] });

case 2:

  • current: ['loan_start_date', '2020-01-01']
  • newFilter: ['loan_start_date', '2021-02-02']
  • call this.props.updateQueryState({ filters: [ ['loan_start_date', '2020-01-01'], ['loan_start_date', '2021-02-02'] ] });

In any case, we always pass the previous value to remove it, and eventually if a new one is set, add it.

onDateChange = newFilter => {
      const [name, value] = newFilter;
      const newFilters = this.props.currentQueryState.filters

      // if there is a new date, add it to the current filters so that the previous date is removed and the new one is added.
      // if the date is cleared, then just leave the previous date in the filters so that it will be remove
      const hasSelectedNewDate = !_isEmtpy(value);
      if (hasSelectedNewDate) {
          newFilters.push(newFilter)
      }

      return this.props.updateQueryState({ filters: newFilters });
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ntarocco I see what you mean and I am sorry for the confusion it was very messy I cleaned it up according to your suggestions, thanks!

topless added 2 commits April 4, 2020 15:04
- filters  loans_from_date and loans_to_date on loan start date
- datepickers for ui
- depends on react-searchkit release 0.18.0
- closes inveniosoftware#168
- converted filters to post_filters
- config expects an axios property
- tests
@ntarocco ntarocco merged commit 76a1d1e into inveniosoftware:master Apr 4, 2020
@topless topless deleted the 168-date-filter branch April 4, 2020 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Req] Statistics - Loans
4 participants